chore: add full support for NLB log #44109
Conversation
Signed-off-by: Kavindu Dodanduwa <[email protected]>
constanca-m
left a comment
constanca-m
left a comment
There was a problem hiding this comment.
Fields look OK, just a comment about ignoring -
| change_type: enhancement | ||
|
|
||
| # The name of the component, or a single word describing the area of concern, (e.g. receiver/filelog) | ||
| component: extension/encoding |
There was a problem hiding this comment.
| component: extension/encoding | |
| component: extension/awslogs_encoding |
| @@ -1 +1,2 @@ | |||
| tls 2.0 2018-12-20T02:59:40 net/my-network-loadbalancer/c6e77e28c25b2234 g3d4b5e8bb8464cd 72.21.218.154:51341 172.100.100.185:443 5 2 98 246 - arn:aws:acm:us-east-2:671290407336:certificate/2a108f19-aded-46b0-8493-c63eb1ef4a99 - ECDHE-RSA-AES128-SHA tlsv12 - my-network-loadbalancer-c6e77e28c25b2234.elb.us-east-2.amazonaws.com - - - 2018-12-20T02:59:30 | |||
There was a problem hiding this comment.
Why? Comparing the two logs, this only contains -
There was a problem hiding this comment.
These are the two examples from official AWS guide - https://docs.aws.amazon.com/elasticloadbalancing/latest/network/load-balancer-access-logs.html
IMO it's good to have them both here as one shows "TLS listener without an ALPN policy" & other with "TLS listener with an ALPN policy"
| - key: aws.elb.incoming_tls_alert | ||
| value: | ||
| stringValue: '-' | ||
| - key: aws.elb.tls_named_group | ||
| value: | ||
| stringValue: '-' |
There was a problem hiding this comment.
Please don't include this -. This should be ignored.
There was a problem hiding this comment.
I will cross check this among other signals and see what's needed
There was a problem hiding this comment.
Yes some checks were missing. I updated to fix this with 4b4d2ea
I have added unset check based on official AWS documentation of the NLB log - https://docs.aws.amazon.com/elasticloadbalancing/latest/network/load-balancer-access-logs.html#access-log-entry-format
extension/encoding/awslogsencodingextension/internal/unmarshaler/elb-access-log/unmarshaler.go
Show resolved
Hide resolved
Signed-off-by: Kavindu Dodanduwa <[email protected]>
constanca-m
left a comment
constanca-m
left a comment
There was a problem hiding this comment.
Thanks, looks much better, but there is still one issue in the code ⏬
| - key: aws.elb.tls_connection_creation_time | ||
| value: | ||
| stringValue: ',"http/1.1"' |
There was a problem hiding this comment.
This doesn't seem right. Can you check @Kavindu-Dodan ?
Description for this field is:
tls_connection_creation_time: The time recorded at the beginning of the TLS connection, in ISO 8601 format.
So something must be off in the code
There was a problem hiding this comment.
The bug is in the extractFields function. This is going to be a bit of a headache to fix... Which also reminds me, this function needs a unit test
4 participants
Description
Adds full field support for NLB log fields. The table below summarizes NLB field and OTel attribute it ends up (extracted from the updated documentation),
aws.elb.connection_timeaws.elb.tls_handshake_timeaws.elb.incoming_tls_alertaws.elb.chosen_cert_arnaws.elb.chosen_cert_serialaws.elb.tls_named_groupaws.elb.alpn_fe_protocolaws.elb.alpn_be_protocolaws.elb.alpn_client_preference_listaws.elb.tls_connection_creation_timeLink to tracking issue
Fixes #43757
Testing
Unit tests already covered field parsing. But I have added both samples from official AWS NLB documentation at https://docs.aws.amazon.com/elasticloadbalancing/latest/network/load-balancer-access-logs.html#access-log-entry-format
Documentation
Documentation updated with the new attributes